Redesign Email Server settings + managed domain flow#1373
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a backend helper to demote other applied managed email domains, updates onboarding to create domains as pending then asynchronously verify them, and refactors the dashboard domain settings into a provider-card UX with a multi-step ManagedDomainSetupDialog. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / Dashboard
participant Dashboard as Dashboard UI
participant Backend as Backend API
participant DB as Database
User->>Dashboard: Choose provider & click "Use this domain"
Dashboard->>Backend: POST /apply-managed-domain (domainId)
Backend->>DB: SELECT APPLIED domains WHERE tenancyId = X AND id != keepId
DB-->>Backend: return list
Backend->>DB: UPDATE set status=VERIFIED, appliedAt=NULL, updatedAt=now() FOR each
DB-->>Backend: updated
Backend->>DB: UPDATE chosen domain set status=APPLIED, appliedAt=now()
DB-->>Backend: updated
Backend-->>Dashboard: 200 OK
Dashboard->>User: show active domain
sequenceDiagram
participant Onboarding as Onboarding Flow
participant Backend as Backend API
participant DB as Database
participant Webhook as Webhook Handler
Onboarding->>Backend: POST /create-managed-domain (status=pending_verification)
Backend->>DB: INSERT domain(status=pending_verification)
DB-->>Backend: created domain
Backend->>Backend: schedule async (1s) invoke
Backend->>Webhook: updateManagedEmailDomainWebhookStatus(domainId, providerStatusRaw=verified)
Webhook->>DB: UPDATE domain set status=verified, providerStatusRaw=verified
DB-->>Webhook: updated
Webhook-->>Backend: confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR redesigns the Email Server settings page with a four-card provider picker, a stepper dialog for managed-domain onboarding, and a DNS records table with copy buttons. It also fixes two backend bugs: multiple domains could remain
Confidence Score: 4/5Safe to merge after addressing the missing transaction wrapping the demote + mark pair in applyManagedEmailProvider. One P1 finding — the demote and mark operations are not atomic, leaving a race window that reproduces the bug being fixed under concurrent load. The two P2 findings (stale emailConfig after domain apply, dropped SMTP field-level validation) are non-blocking UX issues. apps/backend/src/lib/managed-email-onboarding.tsx — the applyManagedEmailProvider function needs its demote + mark pair wrapped in a Prisma $transaction. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User (Dashboard)
participant DS as DomainSettings
participant SDK as stackAdminApp SDK
participant BE as Backend API
U->>DS: Click "Add domain"
DS->>DS: Open ManagedDomainSetupDialog (stage 1)
U->>DS: Enter subdomain + sender, click Continue
DS->>SDK: setupManagedEmailProvider(subdomain, senderLocalPart)
SDK->>BE: POST /managed-email/setup
BE-->>SDK: { domainId, nameServerRecords, status: pending_verification }
SDK-->>DS: SetupState
DS->>DS: Advance to stage 2 (DNS records table)
U->>DS: Click "Check verification"
DS->>SDK: checkManagedEmailStatus(domainId)
SDK->>BE: GET /managed-email/status
BE-->>SDK: { status: verified }
SDK-->>DS: status
DS->>DS: Advance to stage 3 (Domain verified)
U->>DS: Click "Use this domain"
DS->>SDK: applyManagedEmailProvider(domainId)
SDK->>BE: POST /managed-email/apply
BE->>BE: demoteOtherAppliedManagedEmailDomains (query 1)
BE->>BE: markManagedEmailDomainApplied (query 2)
Note over BE: No transaction — race window between queries
BE-->>SDK: { status: applied }
SDK-->>DS: success
DS->>DS: toast + close dialog
DS->>SDK: listManagedEmailDomains (refreshDomains)
SDK-->>DS: updated domain list
DS->>DS: Re-render list (emailConfig not re-fetched)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx (3)
316-324:toastfor "DNS not yet propagated" is borderline — consider an inline note instead.The user just took an explicit "Check verification" action and the dialog stays open on stage 2. Per the repo guideline ("For blocking alerts and errors, never use
toast, as they are easily missed"), this isn't an error, sotoastis allowed — but the result of the action is important feedback and is easy to miss, especially if the toaster is out of the user's eye path while they're focused on the modal. Consider rendering it inline (e.g. near the DNS table, similar to howerroris displayed) so the feedback stays within the modal's flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx around lines 316 - 324, Replace the transient toast call used for the "DNS not yet propagated" case with an inline, persistent message rendered inside the modal (near the DNS records table) so the user sees the result while the dialog remains open: in the branch that currently calls toast (inside the function handling the verification response where next.status is neither "verified" nor "failed"), stop calling toast and instead set a new piece of component state (e.g., add/use an info state like dnsInfoMessage via setDnsInfoMessage) or reuse the existing error rendering pattern, then render that message conditionally next to the DNS table (similar to how setError / error is displayed) so the feedback remains visible while stage 2 is active. Ensure setStage and setError logic is unchanged for the other branches.
790-823: Provider cards usetransition-allwhich animates hover-enter too.Same guideline as above —
transition-allon these provider tiles will run on hover-enter and hover-leave. Prefer scoping to the properties you actually animate (border/background/ring) and disabling on hover so only the exit fades.♻️ Proposed tweak
- "relative text-left rounded-xl border p-4 transition-all", + "relative text-left rounded-xl border p-4 transition-[background-color,border-color,box-shadow] hover:transition-none",As per coding guidelines: "AVOID hover-enter transitions; use only hover-exit transitions. For example,
transition-colors hover:transition-none".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx around lines 790 - 823, The provider card button currently uses "transition-all" which triggers hover-enter animations; update the className on the JSX <button> (the button rendered for each provider where handleSelectProvider(p.value) is used and variables like isSaved/isDraft/isSelected are applied) to scope transitions to only the properties you animate (e.g., border, background, ring, shadow) and disable hover-enter by adding hover:transition-none; for example replace "transition-all" with a scoped transition like "transition-colors transition-shadow" and append "hover:transition-none" so only hover-exit/fade runs while keeping the rest of the conditional classes (isSaved/isDraft/isSelected) intact.
219-239: Minor:setTimeouthas no unmount cleanup and the hover transition applies on enter.Two small things in this button:
- If the dialog/card unmounts within the 1.2 s window after a click,
setCopied(false)runs on an unmounted component (React will warn and the state update is wasted). Consider tracking the timeout id in a ref and clearing it in auseEffectcleanup, or gating thesetCopiedcall with anisMountedref.- Per the repo guideline on hover transitions ("AVOID hover-enter transitions; use only hover-exit transitions. For example,
transition-colors hover:transition-none"), the baretransition-colorshere animates both directions. Addhover:transition-noneso only the exit fades.♻️ Proposed tweak
- className="shrink-0 p-1 rounded-md hover:bg-foreground/[0.06] text-muted-foreground hover:text-foreground transition-colors" + className="shrink-0 p-1 rounded-md hover:bg-foreground/[0.06] text-muted-foreground hover:text-foreground transition-colors hover:transition-none"As per coding guidelines: "AVOID hover-enter transitions; use only hover-exit transitions. For example,
transition-colors hover:transition-none".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx around lines 219 - 239, The CopyButton component sets a timeout without cleanup and uses a hover-enter transition; fix it by tracking the timeout id in a ref (e.g., timeoutRef via useRef) and clearing it on unmount in a useEffect cleanup (or gate setCopied with an isMounted ref) so setCopied(false) won't run after unmount, and update the button's className to include hover:transition-none alongside transition-colors (e.g., "transition-colors hover:transition-none") to avoid hover-enter animations; the key symbols to change are CopyButton, setCopied/setTimeout, timeoutRef/useEffect (or isMounted ref), and the button className.apps/backend/src/lib/managed-email-onboarding.tsx (1)
645-649: Non-atomic demote + mark-applied can transiently leave zero APPLIED domains.
demoteOtherAppliedManagedEmailDomainsruns, thenmarkManagedEmailDomainApplied. If the process dies (or the second query errors) between these statements, the tenancy has no row withstatus = 'APPLIED'even thoughsaveManagedEmailProviderConfighas already rewritten the config to point at the new subdomain. The user-visible display still works (it matches by subdomain), but the DBAPPLIEDstatus becomes misleading for any code that queries it directly.Two safer options:
- Swap the order — mark the new domain
APPLIEDfirst, then demote siblings. Between the statements you transiently have two APPLIED rows, but never zero, and the invariant "at least one APPLIED whenever the config points at a managed subdomain" holds.- Wrap both raw SQL updates in a
prisma.$transaction([...])so the status transition is atomic.Option 2 is strictly safer; option 1 is a one-line change. Feel free to defer if this is acceptable given the config is the source of truth for active sender.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/lib/managed-email-onboarding.tsx` around lines 645 - 649, Wrap the demote+mark operations in a single DB transaction to avoid a transient state with zero APPLIED rows: instead of sequentially awaiting demoteOtherAppliedManagedEmailDomains(...) then markManagedEmailDomainApplied(...), execute their underlying update queries inside prisma.$transaction([...]) (or refactor those functions to return the Prisma query promises and call prisma.$transaction([demotePromise, markPromise])). Ensure you pass tenancyId and domain.id into the transaction calls so both updates run atomically; if refactoring is needed, make demoteOtherAppliedManagedEmailDomains and markManagedEmailDomainApplied return the Prisma query (not perform the await themselves) so they can be combined in the transaction.apps/backend/src/lib/managed-email-domains.tsx (1)
184-198: Prefer$executeRawfor non-returning UPDATE.Since this query returns no rows,
$executeRaw(or$executeRawUnsafe) is the more idiomatic Prisma API for a write with no result set — it returns the affected row count and avoids the implicit unknown array allocation from$queryRaw. Functionally equivalent here, so feel free to defer.♻️ Proposed change
- await globalPrismaClient.$queryRaw(Prisma.sql` + await globalPrismaClient.$executeRaw` UPDATE "ManagedEmailDomain" SET "status" = 'VERIFIED'::"ManagedEmailDomainStatus", "appliedAt" = NULL, "updatedAt" = CURRENT_TIMESTAMP WHERE "tenancyId" = ${options.tenancyId} AND "id" <> ${options.keepId} AND "status" = 'APPLIED'::"ManagedEmailDomainStatus" - `); + `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/lib/managed-email-domains.tsx` around lines 184 - 198, Replace the use of globalPrismaClient.$queryRaw in demoteOtherAppliedManagedEmailDomains with the non-returning variant $executeRaw (or $executeRawUnsafe) since the UPDATE does not return rows; update the call to globalPrismaClient.$executeRaw(Prisma.sql`...`) while keeping the same SQL and interpolated options.tenancyId and options.keepId so the query executes and returns the affected row count instead of an unused result array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx:
- Around line 883-921: The "Use this domain" button is a no-op for rows with
status "applied" that aren't active because isReadyButUnused includes "applied"
but applyManagedEmailProvider short-circuits on applied; restrict the button to
only show for d.status === "verified" by changing the isReadyButUnused
definition (and the JSX condition that renders the DesignButton) to exclude
"applied", and instead render a non-action indicator (e.g., a "Stale/Applied"
label or a "Reapply" link that performs a full reapply flow) for rows where
d.status === "applied" but d.domainId !== activeManagedDomainId so users don’t
get a misleading success toast; update any toast text or handler wrapped by
runAsynchronouslyWithAlert accordingly.
---
Nitpick comments:
In `@apps/backend/src/lib/managed-email-domains.tsx`:
- Around line 184-198: Replace the use of globalPrismaClient.$queryRaw in
demoteOtherAppliedManagedEmailDomains with the non-returning variant $executeRaw
(or $executeRawUnsafe) since the UPDATE does not return rows; update the call to
globalPrismaClient.$executeRaw(Prisma.sql`...`) while keeping the same SQL and
interpolated options.tenancyId and options.keepId so the query executes and
returns the affected row count instead of an unused result array.
In `@apps/backend/src/lib/managed-email-onboarding.tsx`:
- Around line 645-649: Wrap the demote+mark operations in a single DB
transaction to avoid a transient state with zero APPLIED rows: instead of
sequentially awaiting demoteOtherAppliedManagedEmailDomains(...) then
markManagedEmailDomainApplied(...), execute their underlying update queries
inside prisma.$transaction([...]) (or refactor those functions to return the
Prisma query promises and call prisma.$transaction([demotePromise,
markPromise])). Ensure you pass tenancyId and domain.id into the transaction
calls so both updates run atomically; if refactoring is needed, make
demoteOtherAppliedManagedEmailDomains and markManagedEmailDomainApplied return
the Prisma query (not perform the await themselves) so they can be combined in
the transaction.
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx:
- Around line 316-324: Replace the transient toast call used for the "DNS not
yet propagated" case with an inline, persistent message rendered inside the
modal (near the DNS records table) so the user sees the result while the dialog
remains open: in the branch that currently calls toast (inside the function
handling the verification response where next.status is neither "verified" nor
"failed"), stop calling toast and instead set a new piece of component state
(e.g., add/use an info state like dnsInfoMessage via setDnsInfoMessage) or reuse
the existing error rendering pattern, then render that message conditionally
next to the DNS table (similar to how setError / error is displayed) so the
feedback remains visible while stage 2 is active. Ensure setStage and setError
logic is unchanged for the other branches.
- Around line 790-823: The provider card button currently uses "transition-all"
which triggers hover-enter animations; update the className on the JSX <button>
(the button rendered for each provider where handleSelectProvider(p.value) is
used and variables like isSaved/isDraft/isSelected are applied) to scope
transitions to only the properties you animate (e.g., border, background, ring,
shadow) and disable hover-enter by adding hover:transition-none; for example
replace "transition-all" with a scoped transition like "transition-colors
transition-shadow" and append "hover:transition-none" so only hover-exit/fade
runs while keeping the rest of the conditional classes
(isSaved/isDraft/isSelected) intact.
- Around line 219-239: The CopyButton component sets a timeout without cleanup
and uses a hover-enter transition; fix it by tracking the timeout id in a ref
(e.g., timeoutRef via useRef) and clearing it on unmount in a useEffect cleanup
(or gate setCopied with an isMounted ref) so setCopied(false) won't run after
unmount, and update the button's className to include hover:transition-none
alongside transition-colors (e.g., "transition-colors hover:transition-none") to
avoid hover-enter animations; the key symbols to change are CopyButton,
setCopied/setTimeout, timeoutRef/useEffect (or isMounted ref), and the button
className.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80182ca7-2fbd-4f75-9bda-8f35211db4af
⛔ Files ignored due to path filters (2)
apps/dashboard/public/assets/resend-icon-black.svgis excluded by!**/*.svgapps/dashboard/public/assets/resend-icon-white.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
apps/backend/src/lib/managed-email-domains.tsxapps/backend/src/lib/managed-email-onboarding.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx
The mock onboarding now inserts domains as pending_verification and flips them to verified ~1s later (mirroring the real Resend webhook flow), so the test now asserts the initial pending_verification state and waits before expecting verified.
|
Promptless prepared a documentation update related to this change. Triggered by PR #1373 Updated the email documentation to reflect the redesigned Email Server settings UI, including the new visual provider picker with four card options, managed domain stepper flow, DNS records table format, domain status states, and Resend provider option. Review: Document managed email provider |
2 participants
Summary
Rewrites the Email Server section of the project email settings page and the managed-domain setup flow. Replaces the dropdown + conditional-fields layout with a visual four-card picker, a clearer unsaved-state model, a stepper dialog for managed-domain onboarding, and a consistent tracked-domains list. Also fixes two data-correctness bugs in the managed-domain backend.
Walkthrough (2×, dead-frames trimmed)
Before
The saved state was a minimal dropdown, but choosing Custom SMTP / Resend revealed a long conditional form with a hidden gear toggle for server config, no clear "what is saved" signal, and a separate dialog pattern for managed domains.
After — Provider cards
Four visual cards (Stack Shared, Managed Domain, Resend, Custom SMTP) with updated copy. The saved provider shows a green Current pill; the card the user is previewing shows an amber dashed Draft pill. An amber unsaved-changes banner appears between the picker and the form when state diverges from saved, so it is unambiguous that a click is not yet committed.
Copy changes:
apps/dashboard/public/assets/)After — Managed domain list + stepper dialog
Selecting Managed Domain immediately shows the tracked-domain list with an Add domain button. Each row reflects real status (Active / Verified / Waiting for DNS / Verifying / Failed). Exactly one domain can be Active — the one matching the saved email config; every other verified/applied domain shows a Use this domain button so switching is always possible.
Adding a domain opens a 3-stage dialog with a horizontal stepper (Verify is right-aligned for the final step). Stage 2 replaces the old bare NS-list with a proper Type / Name / Content DNS records table with per-row copy buttons.
Bug fixes
APPLIEDeven though only one could be in the saved config. New helperdemoteOtherAppliedManagedEmailDomains({ tenancyId, keepId })runs insideapplyManagedEmailProviderto demote all other applied rows in the tenancy back toVERIFIEDbefore marking the new one.status === verified. A domain that had been applied then replaced could never be re-applied from the UI. Button now appears for anyverifiedorappliedrow that is not currently in use; the Active label is derived from config match instead of DB status.shouldUseMockManagedEmailOnboarding()used to insert domains asverifiedsynchronously. Now the domain is created aspending_verification, and a fire-and-forgetrunAsynchronously(() => wait(1000))updates it toverified— mirroring the real Resend webhook flow so the UI states (pending → verifying → verified) are exercised in local dev.Test plan
Draftpill + amber banner; Discard restores; Save commits and flipsCurrentto the new cardpnpm typecheck(dashboard + backend) andpnpm lintpassSummary by CodeRabbit
New Features
Bug Fixes
Tests